-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: get repo service for specific commit #479
feat: get repo service for specific commit #479
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #479 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 412 412
Lines 34187 34150 -37
=======================================
- Hits 33252 33217 -35
+ Misses 935 933 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #479 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 412 412
Lines 34187 34150 -37
=======================================
- Hits 33252 33217 -35
+ Misses 935 933 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #479 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 412 412
Lines 34187 34150 -37
=======================================
- Hits 33252 33217 -35
+ Misses 935 933 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #479 +/- ##
==========================================
+ Coverage 97.29% 97.30% +0.01%
==========================================
Files 443 443
Lines 34916 35102 +186
==========================================
+ Hits 33972 34157 +185
- Misses 944 945 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes
|
ccc968c
to
1ed190e
Compare
Change the repository service so we have a function that checks whether a commit has been pinned to a specific app and returns the TorngitAdapter with said app.
1ed190e
to
b8dcb6f
Compare
if installation_for_commit is None: | ||
return get_repo_provider_service(repository, fallback_installation_name) | ||
|
||
ghapp_details = get_specific_github_app_details( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the logic inside here,
worker/services/bots/github_apps.py
Line 163 in b8dcb6f
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM. I'm a bit confused/concerned about all the GitHub specific stuff in the repository
, but it appears that other functions in that file is referring to GitHub specific stuff, so I'll let it go. Just a few comments/questions.
services/repository.py
Outdated
commit: Commit, | ||
fallback_installation_name: str = GITHUB_APP_INSTALLATION_DEFAULT_NAME, | ||
) -> torngit.base.TorngitBaseAdapter: | ||
"""Gets a Torngit adapter (potentially) using a specific github app as the authentication source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Gets a Torngit adapter (potentially) using a specific github app as the authentication source. | |
"""Gets a Torngit adapter (potentially) using a specific GitHub app as the authentication source. |
services/repository.py
Outdated
@@ -65,7 +97,9 @@ def get_repo_provider_service( | |||
data = TorngitInstanceData( | |||
repo=RepoInfo( | |||
name=repository.name, | |||
using_integration=_is_repo_using_integration(repository), | |||
using_integration=( | |||
adapter_auth_info["selected_installation_info"] is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will raise a KeyError
if selected_installation_info
is not found. If we don't want to error, then adapter_auth_info.get("selected_installation_info")
is safer.
if installation_for_commit is None: | ||
return get_repo_provider_service(repository, fallback_installation_name) | ||
|
||
ghapp_details = get_specific_github_app_details( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do a check in get_repo_provider_service
to ensure that the repo is a GitHub repo. Should we do a check here as well? This will save us a database read to the GitHub App Installation table I think.
In fact, many parts of this function is GitHub specific... can we skip some more steps below if it's not a GitHub commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is in get_github_app_for_commit.
From the docstring: "If the commit doesn't have a particular app assigned to it, return regular get_repo_provider_service
choice".
Because apps only exist for GitHub it follows that only if it's a GitHub repo the code would get to this point.
Change the repository service so we have a function that
checks whether a commit has been pinned to a specific app
and returns the TorngitAdapter with said app.
Note on
services.repository._is_repo_using_integration
: this function has become effectively obsolete since the introduction ofget_adapter_auth_information
because we can tell if we are using the installation by the presence of "installation_info" or not.👀 This is PR 3/4 in the pinning commits to apps endeavour
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.